Drop recursively routed packets

On certain OSes (Windows, OS X) when network adapter is disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled), operating system starts to use tun as an external interface. Outgoing packets are routed to tun, UDP encapsulated, given to routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power, traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as destination address.

Tested on Win7/10, OS X.

Trac #642

---
 src/openvpn/forward.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

index 36a99e6..05445a1 100644
@@ -973,6 +973,68 @@ read_incoming_tun (struct context *c)
   perf_pop ();
 }
 
+/**
+ * Drops UDP packets which OS decided to route via tun. 
+ * 
+ * On Windows and OS X when netwotk adapter is disabled or
+ * disconnected, platform starts to use tun as external interface. 
+ * When packet is sent to tun, it comes to openvpn, encapsulated
+ * and sent to routing table, which sends it again to tun.
+ */
+static void
+drop_recursive (struct context *c, struct buffer *buf)
+{
+  bool drop = false;
+  struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;
+
+  if (is_ipv4 (TUNNEL_TYPE (c->c1.tuntap), buf))
+    {
+      const struct openvpn_iphdr *pip;
+
+      /* make sure we got whole IP header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_iphdr))
+	return;
+
+      /* skip ipv4 packets for ipv6 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET)
+	return;
+
+      pip = (struct openvpn_iphdr *) BPTR (buf);
+
+      /* drop packets with same dest addr as gateway */
+      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
+	drop = true;
+    }
+  else if (is_ipv6 (TUNNEL_TYPE (c->c1.tuntap), buf))
+    {
+      const struct openvpn_ipv6hdr *pip6;
+
+      /* make sure we got whole IPv6 header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_ipv6hdr))
+	return;
+
+      /* skip ipv6 packets for ipv4 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET6)
+	return;
+
+      /* drop packets with same dest addr as gateway */
+      pip6 = (struct openvpn_ipv6hdr *) BPTR(buf);
+      if (IN6_ARE_ADDR_EQUAL(&tun_sa.addr.in6.sin6_addr, &pip6->daddr))
+	drop = true;
+    }
+
+  if (drop)
+    {
+      struct gc_arena gc = gc_new ();
+
+      c->c2.buf.len = 0;
+
+      msg(D_LOW, "Recursive routing detected, drop tun packet to %s",
+		print_link_socket_actual(c->c2.to_link_addr, &gc));
+      gc_free (&gc);
+    }
+}
+
 /*
  * Input:  c->c2.buf
  * Output: c->c2.to_link

@@ -998,6 +1060,7 @@ process_incoming_tun (struct context *c)
 
   if (c->c2.buf.len > 0)
     {
+      drop_recursive (c, &c->c2.buf);
       /*
        * The --passtos and --mssfix options require
        * us to examine the IP header (IPv4 or IPv6).

1.9.1

v2: better method naming

On certain OSes (Windows, OS X) when network adapter is disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled), operating system starts to use tun as an external interface. Outgoing packets are routed to tun, UDP encapsulated, given to routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power, traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as destination address.

Tested on Win7/10, OS X.

Trac #642

---
 src/openvpn/forward.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

index 36a99e6..af05bd0 100644
@@ -973,6 +973,68 @@ read_incoming_tun (struct context *c)
   perf_pop ();
 }
 
+/**
+ * Drops UDP packets which OS decided to route via tun. 
+ * 
+ * On Windows and OS X when netwotk adapter is disabled or
+ * disconnected, platform starts to use tun as external interface. 
+ * When packet is sent to tun, it comes to openvpn, encapsulated
+ * and sent to routing table, which sends it again to tun.
+ */
+static void
+drop_if_recursive_routing (struct context *c, struct buffer *buf)
+{
+  bool drop = false;
+  struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;
+
+  if (is_ipv4 (TUNNEL_TYPE (c->c1.tuntap), buf))
+    {
+      const struct openvpn_iphdr *pip;
+
+      /* make sure we got whole IP header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_iphdr))
+	return;
+
+      /* skip ipv4 packets for ipv6 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET)
+	return;
+
+      pip = (struct openvpn_iphdr *) BPTR (buf);
+
+      /* drop packets with same dest addr as gateway */
+      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
+	drop = true;
+    }
+  else if (is_ipv6 (TUNNEL_TYPE (c->c1.tuntap), buf))
+    {
+      const struct openvpn_ipv6hdr *pip6;
+
+      /* make sure we got whole IPv6 header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_ipv6hdr))
+	return;
+
+      /* skip ipv6 packets for ipv4 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET6)
+	return;
+
+      /* drop packets with same dest addr as gateway */
+      pip6 = (struct openvpn_ipv6hdr *) BPTR(buf);
+      if (IN6_ARE_ADDR_EQUAL(&tun_sa.addr.in6.sin6_addr, &pip6->daddr))
+	drop = true;
+    }
+
+  if (drop)
+    {
+      struct gc_arena gc = gc_new ();
+
+      c->c2.buf.len = 0;
+
+      msg(D_LOW, "Recursive routing detected, drop tun packet to %s",
+		print_link_socket_actual(c->c2.to_link_addr, &gc));
+      gc_free (&gc);
+    }
+}
+
 /*
  * Input:  c->c2.buf
  * Output: c->c2.to_link

@@ -998,6 +1060,7 @@ process_incoming_tun (struct context *c)
 
   if (c->c2.buf.len > 0)
     {
+      drop_if_recursive_routing (c, &c->c2.buf);
       /*
        * The --passtos and --mssfix options require
        * us to examine the IP header (IPv4 or IPv6).

1.9.1

Hi,

Coming back to this patch set - could you share your test results?

I'll then go and see that I can code review & merge during the next week.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Tested on Windows 10 and Linux. Works as intended.
I've connected to the VPN which redirects the gateway and physically unplugged Ethernet cable. Git version started to consume 100% CPU while patched version either doesn't consume CPU and doesn't print anything or doesn't consume CPU and print 5-10 messages per second to the log with verb 4.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Can this happen on Linux or *BSD? Could this recursive routing actually happen at all?

[...snip...]

[...snip...]

How likely is it that these two checks will hit? Do we truly need them? I'm basically concerned of the potential cost an extra check adds. As the link speed increases, such checks will have an impact later on if now. If truly needed, is it worth looking into providing some branch prediction hints to the compiler? (unlikely/likely hints)

If this is truly only an issue on Windows and OSX, can we consider to #ifdef the drop_if_recursive_routing() call?

- --
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAleuFGMACgkQDC186MBRfrphqACcCJ7UNTcaFM79njJ1HXFpw7hX NbcAn2MgNCVh1joOOKhPo46dJ+5GAGdq
=xaHi
-----END PGP SIGNATURE-----

Hi,

Of course :-) - the moment you lose your external interface, for example, because you unplug the network cable and network manager deconfigures ip address plus all routes via that interface...

What is left is "routes into the tunnel", so you send the tunneled packets into the tunnel again. Without that patch, this leads to "openvpn eats 100% cpu, your VPN is down, and until ping-timeout fires, it won't recover and the user has no idea why it is broken".

Very typical problem with routing and tunnels :-)

It's quite likely for IPv6 packets to be transported over an IPv4 tunnel, and vice versa, and will be even *more* likely for the next 10 years to come.

So comparing "inside IPv6 address" to "outside IPv4 address" needs to be avoided (at best, the code might misfire, at worst, you might overrun comparing an IPv6 address to something that has not allocated enough bytes) - maybe it can be written more efficient, but in the end you always have the "is it the same protocol inside and outside?" check, before you go on comparing addresses.

Right now, the code is nicely readable by using the is_ipv4() macro etc., but sacrificing this by having a check very early on that is the unrolled form of the macro, and basically does

if ( outside_address_family != inside_address_family ) { return; }

would indeed be more efficient for the mixed-family case.

OTOH, you need another comparison then for "is it IPv4? is it IPv6?", so the overall amount of code for the same-family case might be even higher.

OTOH again, is_ipv4() boils down to an amazing amount of code... so moving *that* from proto.c to proto.h and make it an inline function (and hope that the compiler will be smart enough to keep track of all the then-duplicate checks for is_ipv4() vs. is_ipv6() and just optimize it properly) might produce more efficient *and* more readable code :-) (and since it's only called in two places ever, the extra code size due to inlining isn't big)

For the "protocol variants", you can't, because it depends on what users do with their tunnels. Today, they might be predominantly IPv4-over-IPv4, while my personal sessions are mostly IPv6-over-IPv4 already...

It can happen on all platforms where "inside" and "outside" routing (wrt openvpn) happens in the same routing context / namespace / ... and "some external entity" can modify the "outside" routing table without OpenVPN knowing. In other words: on all platforms we support today.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

I could probably add that patch has been in our production for a while (several months), OS X / Win, works as expected.

-Lev

2016-08-13 12:58 GMT+03:00 Gert Doering <[email protected]...>:

-Lev

Hi,

ACK.

Thanks for coding this, thanks to Valdikss for testing (David, can you include the tested-by: in the commit?).

Code still looks good, as in "does what is advertised, and safely does so".

Per discussion with David, we're not totally happy with the performance implications - but that's not your fault, our is_ipv4() and is_ipv6() functions are really doing their job in a very non-optimized way (read:
all the work is done twice, once to find out whether it's v4, then again to find out whether it's v6).

I volunteer to refactor the two places where is_ipv*() are called today (thinking along the lines of "ip_version = get_ip_version(...)" and then compare ip_version to "4" or "6" - plus, making this an inline function)

For now, we stick to "fixes the issue in a easy to read and non-intrusive way", so it can go into 2.3.12.

thanks again :)

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

[..]

As stupid as this feels - we need to back this out again, because it breaks TAP mode. Buildbot complained that all tap tests failed, and manually bisecting master nailed it to *this* patch, and release/2.3 is similarily broken.

On the server side, for a t_client test with --dev tap, with this patch, you see "the source mac is rotating"

Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:d8:7a -> cron2-gentoo-i386/193.xx.xx.xx
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:b9:40:01:d7:c1 -> cron2-gentoo-i386/193.xx.xx.xx
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: bad source address from client [01:72:40:01:fc:a0], packet dropped
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:3e:b1 -> cron2-gentoo-i386/193.xx.xx.xx
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:b9:40:01:3d:f8 -> cron2-gentoo-i386/193.xx.xx.xx
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: bad source address from client [01:72:40:01:62:d7], packet dropped
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:d7:d8 -> cron2-gentoo-i386/193.xx.xx.xx

... which looks like "the IP header ends up where the ethernet header should be" (every ping packet shows up as "new source address" on the openvpn server).

I have no idea what this could be, but since we want 2.3.12 out *today*, we'll need to back it out of 2.3 for the time being.

Lev, do you have time to investigate?

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

is_ipv_X() in proto.c is to blame here, as it breaks the buffer for tap mode...

...
/* IP version is stored in the same bits for IPv4 or IPv6 header */ if (OPENVPN_IPH_GET_VER (ih->version_len) == ip_ver)

    return buf_advance (buf, offset);

...

... and "offset" is only set != 0 in tap mode - so in tun mode, it works, and the code itself looks totally reasonable.

David, please revert both release/2.3 and master, and we'll need to rethink this, not using "is_ipv4()"...

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

Ow, ow, this patch is not good...

Turns out it also breaks p2p mode...

Tue Aug 23 21:28:07 2016 us=528530 UDP link local (bound): [AF_INET][undef]:51198 Tue Aug 23 21:28:07 2016 us=528539 UDP link remote: [AF_INET][undef]:51198
Tue Aug 23 21:28:07 2016 us=528612 TIMER: coarse timer wakeup 1 seconds
Tue Aug 23 21:28:07 2016 us=529094 RANDOM USEC=205433
Tue Aug 23 21:28:07 2016 us=529128 PO_CTL rwflags=0x0001 ev=5 arg=0x006a5ee8
Tue Aug 23 21:28:07 2016 us=529137 PO_CTL rwflags=0x0001 ev=4 arg=0x006a5824
Tue Aug 23 21:28:07 2016 us=529147 I/O WAIT TR|Tw|SR|Sw [1/205433]
Tue Aug 23 21:28:07 2016 us=529958 PO_WAIT[1,0] fd=4 rev=0x00000001 rwflags=0x0001 arg=0x006a5824 Tue Aug 23 21:28:07 2016 us=529976 event_wait returned 1
Tue Aug 23 21:28:07 2016 us=529984 I/O WAIT status=0x0004
Tue Aug 23 21:28:07 2016 us=530017 read from TUN/TAP returned 76
Tue Aug 23 21:28:07 2016 us=530028 TUN READ [76]
Memory fault

... because it unconditionally checks

1005 drop_if_recursive_routing (struct context *c, struct buffer *buf)
1006 {
1007 bool drop = false;
1008 struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;

... which is a NULL ptr dereference here...

(gdb) print c->c2.to_link_addr
$7 = (struct link_socket_actual *) 0x0

(tun interface comes up, operating system wants to send something, most likely IPv6 DAD or the likes, which won't go out due to "no remote yet", but still enters here, and *blam*)

Needs more thorough testing... I'll put the next version into my treadmill of "all possible weird ways to hook stuff together" (p2p, --inetd, ...) and that will hopefully find all issues before pushing it out.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Done.

- --
kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJXvM9gAAoJEIbPlEyWcf3ycNEQALsZ2nJQfHIqYalF4a+xxoA9 63R76nSZG6qQo1uyZnHwipBf0crYEPCLd4EbRdSIV3leilXrg3Va0QkW9X3Wn0ld E5uQ4WR4Y4pNkrSB1+l2Iw+N0b1675H5tH/3pKhLe73RvVjI4/APiYtNMEA2FoUv hnmTBgXzNgPfhJxZgkPiJ4LHIL2XG6SfuFt4H56InIs2BraOul6WKZ4XbvTKpE/R fyRHMg4BCzRI20g5yd4SpXYlcMkggIPckFss8A78MlpeZfwZgRvsVGtviNyuL62o Rodv5QSq0nxEMTqIiOK4hhIxmVBiOZu2Xh4HdnG9AAa3ghkux23fD8Iy+aGKmVvf I2PEZCSorealH19DS8AoIDHaD+dlv8XNz46vwcfxLB06M5JUgTS3mt7iQWXI2dFP 8ORCrllvNb0HZjGPNrobkRGZ4JSos8TCjAYbereRpci642Qy323BqFHljwKbquV0 EThKqdHiQo/x8xJWaqxEz58AATBKF775b4lwy1DCbGO2ca2RSfZJueGzytGtn4Ke d/5B2oLyOVT6vW5guNTvla8eywNKyVHiwAsaQs31tVEyGOjVa7mTQjUbpowVfntg PRy4kC+imeElwHXun9dgVg0q9AwdTOQuUUt4OJUyHJqh8Bdp3wYTvxGO4yAFVyMu yvWuf4Gfwi6FOBpNyjCt
=/Fjb
-----END PGP SIGNATURE-----

Hi,

may I suggest to make this configurable, i.e. the user can specify whether rec routed packets should be dropped? I'm afraid that we might end up with code that drops packets that really should not be dropped - people do weird things with routing: in 99% of the cases in error, but in 1% of the cases because they want to do something funky.

It would also make it easy to include the current code in 2.3 - turn it on in TUN mode by default and OFF in TAP mode.

JM2CW,

JJK

Hi,

Well...

There is no way that this might ever work(*).

If your tunnel gateway is 1.2.3.4, and you send packets to 1.2.3.4 into the tunnel, how are these going to arrive?

Packet goes to TUN, is ecapsulated, and sent to 1.2.3.4.

1.2.3.4 route points to TUN, so packet goes to TUN, gets another layer of openvpn, and is sent to 1.2.3.4.

1.2.3.4 route points to TUN, so packet goes to TUN, gets a *third* layer of openvpn, and is sent to 1.2.3.4.

repeat, until the packet hits 1500 byte MTU, and gets fragmented into *two* packets that loop forever...

gert

(*) it will work if and only if there are multiple routing tables involved, that is, packets sent by openvpn itself are not subject to the routing tables that would move packets *into* the tunnel - so the Android client wouldn't need it (VPN API takes care of this), and with the work on Github #13 to suport network name spaces on linux and multiple routing tables on *BSD, there are indeed scenarios where this makes sense...

... so you're right, v3 needs to have an option...

(This started out really small and simple :-) )

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

So, following changes are required for V3:

1) No drop_if_recursive() call for P2P
2) Same for TAP
3) Add an option to disable it

Sounds reasonable?

2016-08-24 16:13 GMT+03:00 Gert Doering <[email protected]...>:

-Lev

Hi,

Well, sort of. It's useful in p2p mode as well, but it needs to check if we already know the remote address before trying to read via the pointer :-) - basically, this code:

+ struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;

needs to be prefixed by

if ( c->c2.to_link_addr == NULL ) return; /* no remote addr known */

(maybe the same effect can be achieved by moving the drop_if_recursive() call slightly further down in the packet processing, because something should drop it anyway if there is no remote address)

Nah, not "disable it on tap", but "look up the protocol type without messing up the buffer" (so, not using is_ipv4() and is_ipv6()). The function itself is useful on tap as well.

Yes... people using multiple routing tables will not need this check, and it might get in the way. (I'm not a big fan of "yet another option", but it seems necessary)

Yes. Thanks :-) - I'm also willing to work on it, just had no time yet.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

v3: Use better way of figuring out IP proto version which does not break TAP mode. Add an option to allow recursive routing, could be useful when packets sent by openvpn itself are not subject to the routing tables that would move packets into the tunnel.

v2: better method naming

On certain OSes (Windows, OS X) when network adapter is disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled), operating system starts to use tun as an external interface. Outgoing packets are routed to tun, UDP encapsulated, given to routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power, traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as destination address.

Tested on Win7/10, OS X.

---
 Changes.rst           |  2 ++
 doc/openvpn.8         |  4 +++
 src/openvpn/forward.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/options.c | 10 ++++++++
 src/openvpn/options.h |  4 +++
 src/openvpn/proto.h   | 32 +++++++++++++++++++++++
 6 files changed, 123 insertions(+)

index 9fcba75..4cc4acf 100644
@@ -135,6 +135,8 @@ User-visible Changes
   ciphers configured in the config file.  Use --ncp-disable if you don't want
   that.
 
+- On the client side recursively routed packets, which have same destination as host,
+  are dropped. This could be disabled with --allow-recursive-routing option.
 
 Maintainer-visible changes
 --------------------------

index 2f42636..49d2f72 100644
@@ -3985,6 +3985,10 @@ to the same server, with
 
 OpenVPN will not send any exit
 notifications unless this option is enabled.
+.TP
+.B \-\-allow\-recursive\-routing
+When this option is set, OpenVPN will not drop incoming tun packets
+with same destination as host.
 .\"*********************************************************
 .SS Data Channel Encryption Options:
 These options are meaningful for both Static & TLS-negotiated key modes

index c100d4c..90d26c0 100644
@@ -500,6 +500,8 @@ static const char usage_message[] =
   "--connect-timeout n : when polling possible remote servers to connect to\n"
   "                  in a round-robin fashion, spend no more than n seconds\n"
   "                  waiting for a response before trying the next server.\n"
+  "--allow-recursive-routing : When this option is set, OpenVPN will not drop\n"
+  "                  incoming tun packets with same destination as host.\n"
 #endif
 #ifdef ENABLE_OCC
   "--explicit-exit-notify [n] : On exit/restart, send exit signal to\n"

@@ -875,6 +877,7 @@ init_options (struct options *o, const bool init_gc)
   }
 #endif /* WIN32 */
 #endif /* P2MP_SERVER */
+  o->allow_recursive_routing = false;
 }
 
 void

@@ -2128,6 +2131,8 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 	msg (M_USAGE, "--ifconfig-ipv6-pool needs --ifconfig-ipv6");
       if (options->ifconfig_ipv6_local && !options->tun_ipv6 )
 	msg (M_INFO, "Warning: --ifconfig-ipv6 without --tun-ipv6 will not do IPv6");
+      if (options->allow_recursive_routing)
+	msg (M_USAGE, "--allow-recursive-routing cannot be used with --mode server");
 
       if (options->auth_user_pass_file)
 	msg (M_USAGE, "--auth-user-pass cannot be used with --mode server (it should be used on the client side only)");

@@ -7365,6 +7370,11 @@ add_option (struct options *options,
       options->keying_material_exporter_length = ekm_length;
     }
 #endif
+  else if (streq (p[0], "allow-recursive-routing") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->allow_recursive_routing = true;
+    }
   else
     {
       int i;

index 9b7b57c..45412b1 100644
@@ -596,6 +596,10 @@ struct options
 #endif
 
   struct pull_filter_list *pull_filter_list;
+
+  /* Useful when packets sent by openvpn itself are not subject
+     to the routing tables that would move packets into the tunnel. */
+  bool allow_recursive_routing;
 };
 
 #define streq(x, y) (!strcmp((x), (y)))

index f91e787..93444dd 100644
@@ -219,6 +219,38 @@ struct ip_tcp_udp_hdr {
                              - sizeof(struct openvpn_tcphdr))
 
 /*
+ * This returns an ip protocol version of packet inside tun.
+ */
+inline static int get_tun_ip_ver(int tunnel_type, struct buffer *buf)
+{
+  int offset;
+  int ip_ver;
+  const struct openvpn_iphdr *ih;
+
+  ip_ver = -1;
+
+  /* for tun get ip version from ip header */
+  if (tunnel_type == DEV_TYPE_TUN)
+    {
+      if (likely(BLEN (buf) >= (int) sizeof (struct openvpn_iphdr)))
+	{
+	  ip_ver = OPENVPN_IPH_GET_VER (*BPTR(buf));
+	}
+    }
+  else if (tunnel_type == DEV_TYPE_TAP)
+    {
+      /* for tap get ip version from eth header */
+      if (likely(BLEN (buf) >= (int)(sizeof (struct openvpn_ethhdr))))
+	{
+	  const struct openvpn_ethhdr *eh = (const struct openvpn_ethhdr *) BPTR (buf);
+	  return (ntohs (eh->proto) == OPENVPN_ETH_P_IPV6) ? 6 : 4;
+	}
+    }
+
+    return ip_ver;
+}
+
+/*
  * If raw tunnel packet is IPv4 or IPv6, return true and increment
  * buffer offset to start of IP header.
  */

1.9.1

ACK, thanks. Took us long enough.

Tested on Linux, does what it says (--verb 4 shows nice log messages, with --allow-recursive-routing (and UDP), you get the expected CPU explosion instead).

Passes all my t_client tests on client AND server, including --inetd and p2p server.

I had to apply two of your changes manually - the option check in options.c and Changes.rst had changes in the preceding lines due to
other patches, so "git am" refused to apply it "just so". Also,
a small whitespace change was applied to proto.h

Your patch has been applied to the master and release/2.3 branch (2.3 had conflicts in Changes.rst and options.[ch] - easily solved, again by fixing contextual lines).

commit fa3e730e19a71419cb88a021b9a0333f088b39ac (master) commit 7c930767c0b4ae38c4d886678f091a6077ed55a9 (release/2.3)

Author: Lev Stipakov
Date:   Sat Sep 17 12:47:32 2016 +0300

     Drop recursively routed packets

     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12476.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

Bah. I wanted to do everything right, but missed one crucial test: that it "does what it says" in tap mode as well - well, it doesn't.

Unlike v2, this will not break tap (or p2p server or --inetd) mode, but the functionality is not right.

This code:

      /* make sure we got whole IP header */
      if (BLEN (buf) < (int) sizeof (struct openvpn_iphdr))
        return;

      /* skip ipv4 packets for ipv6 tun */
      if (tun_sa.addr.sa.sa_family != AF_INET)
        return;

      pip = (struct openvpn_iphdr *) BPTR (buf);

      /* drop packets with same dest addr as gateway */
      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
        drop = true;

... does not take "if it's TAP, skip the ethernet header" into account, so is comparing tun_sa...s_addr with "something", but not with the IP address in the IP packet inside the ethernet frame.

So, unfortunately, another NAK.

But I noticed before pushing, so just totally ignore my previous mail - this is not in the official tree (and won't make 2.3.13, meh).

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

From: Lev Stipakov <[email protected]...>

v4:
- Account for IP header offset in TAP mode
- Correct handle of non-IP protocols in TAP mode

v3: Use better way of figuring out IP proto version which does not break TAP mode. Add an option to allow recursive routing, could be useful when packets sent by openvpn itself are not subject to the routing tables that would move packets into the tunnel.

v2: better method naming

On certain OSes (Windows, OS X) when network adapter is disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled), operating system starts to use tun as an external interface. Outgoing packets are routed to tun, UDP encapsulated, given to routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power, traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as destination address.

Tested on Win7/10, OS X.

---
 Changes.rst           |  2 ++
 doc/openvpn.8         |  4 +++
 src/openvpn/forward.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/options.c | 10 +++++++
 src/openvpn/options.h |  4 +++
 src/openvpn/proto.h   | 39 ++++++++++++++++++++++++++++
 6 files changed, 131 insertions(+)

index 8fd5859..d7aae00 100644
@@ -188,6 +188,8 @@ User-visible Changes
   capable. The ``--tun-ipv6`` option is ignored (behaves like it is always
   on).
 
+- On the client side recursively routed packets, which have same destination as host,
+  are dropped. This could be disabled with --allow-recursive-routing option.
 
 Maintainer-visible changes
 --------------------------

index 3a4ab21..863dcf9 100644
@@ -4004,6 +4004,10 @@ to the same server, with
 
 OpenVPN will not send any exit
 notifications unless this option is enabled.
+.TP
+.B \-\-allow\-recursive\-routing
+When this option is set, OpenVPN will not drop incoming tun packets
+with same destination as host.
 .\"*********************************************************
 .SS Data Channel Encryption Options:
 These options are meaningful for both Static & TLS-negotiated key modes

index be31ed3..552bf5a 100644
@@ -508,6 +508,8 @@ static const char usage_message[] =
   "--connect-timeout n : when polling possible remote servers to connect to\n"
   "                  in a round-robin fashion, spend no more than n seconds\n"
   "                  waiting for a response before trying the next server.\n"
+  "--allow-recursive-routing : When this option is set, OpenVPN will not drop\n"
+  "                  incoming tun packets with same destination as host.\n"
 #endif
 #ifdef ENABLE_OCC
   "--explicit-exit-notify [n] : On exit/restart, send exit signal to\n"

@@ -886,6 +888,7 @@ init_options (struct options *o, const bool init_gc)
   }
 #endif /* WIN32 */
 #endif /* P2MP_SERVER */
+  o->allow_recursive_routing = false;
 }
 
 void

@@ -2134,6 +2137,8 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 	msg (M_USAGE, "--ifconfig-pool-persist must be used with --ifconfig-pool");
       if (options->ifconfig_ipv6_pool_defined && !options->ifconfig_ipv6_local )
 	msg (M_USAGE, "--ifconfig-ipv6-pool needs --ifconfig-ipv6");
+      if (options->allow_recursive_routing)
+	msg (M_USAGE, "--allow-recursive-routing cannot be used with --mode server");
       if (options->auth_user_pass_file)
 	msg (M_USAGE, "--auth-user-pass cannot be used with --mode server (it should be used on the client side only)");
       if (options->ccd_exclusive && !options->client_config_dir)

@@ -7385,6 +7390,11 @@ add_option (struct options *options,
       options->keying_material_exporter_length = ekm_length;
     }
 #endif
+  else if (streq (p[0], "allow-recursive-routing") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->allow_recursive_routing = true;
+    }
   else
     {
       int i;

index 2f91a52..0ebea30 100644
@@ -597,6 +597,10 @@ struct options
 #endif
 
   struct pull_filter_list *pull_filter_list;
+
+  /* Useful when packets sent by openvpn itself are not subject
+     to the routing tables that would move packets into the tunnel. */
+  bool allow_recursive_routing;
 };
 
 #define streq(x, y) (!strcmp((x), (y)))

index f91e787..922f936 100644
@@ -219,6 +219,45 @@ struct ip_tcp_udp_hdr {
                              - sizeof(struct openvpn_tcphdr))
 
 /*
+ * This returns an ip protocol version of packet inside tun
+ * and offset of IP header (via parameter).
+ */
+inline static int get_tun_ip_ver(int tunnel_type, struct buffer *buf, int *ip_hdr_offset)
+{
+  int ip_ver = -1;
+
+  /* for tun get ip version from ip header */
+  if (tunnel_type == DEV_TYPE_TUN)
+    {
+      *ip_hdr_offset = 0;
+      if (likely(BLEN (buf) >= (int) sizeof (struct openvpn_iphdr)))
+	{
+	  ip_ver = OPENVPN_IPH_GET_VER (*BPTR(buf));
+	}
+    }
+  else if (tunnel_type == DEV_TYPE_TAP)
+    {
+      *ip_hdr_offset = (int)(sizeof (struct openvpn_ethhdr));
+      /* for tap get ip version from eth header */
+      if (likely(BLEN (buf) >= *ip_hdr_offset))
+	{
+	  const struct openvpn_ethhdr *eh = (const struct openvpn_ethhdr *) BPTR (buf);
+	  uint16_t proto = ntohs (eh->proto);
+	  if (proto == OPENVPN_ETH_P_IPV6)
+	    {
+	      ip_ver = 6;
+	    }
+	  else if (proto == OPENVPN_ETH_P_IPV4)
+	    {
+	      ip_ver = 4;
+	    }
+	}
+    }
+
+    return ip_ver;
+}
+
+/*
  * If raw tunnel packet is IPv4 or IPv6, return true and increment
  * buffer offset to start of IP header.
  */

1.9.1

ACK (finally).

Tested with tun+tap for proper recursion drops on v4+v6 (works, though on tap it's sometimes non-trivial to reproduce as you need an valid ARP entry first...), and with all I have for client or server side breakage (tun, tap, p2mp, p2p, --inetd, ...) - no breakage.

Your patch has been applied to the master and release/2.3 branch.

I've taken the liberty to add trac#642 to the commit message, fix an indentation oversight in proto.h, shorten the Changes.rst lines somewhat, and adjust 2.3 Changes.rst to "2.3 style" :-)

commit e8c42658ff8df10ad56659788a73900648b9d92d (master) commit 748974b960a099ef4fdf083f25226659775603b9 (release/2.3)

Author: Lev Stipakov
Date:   Thu Nov 3 23:28:23 2016 +0200

     Drop recursively routed packets

     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12894.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

From: Lev Stipakov <[email protected]...>

v3: Use better way of figuring out IP proto version which does not break TAP mode. Add an option to allow recursive routing, could be useful when packets sent by openvpn itself are not subject to the routing tables that would move packets into the tunnel.

v2: better method naming

On certain OSes (Windows, OS X) when network adapter is disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled), operating system starts to use tun as an external interface. Outgoing packets are routed to tun, UDP encapsulated, given to routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power, traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as destination address.

Tested on Win7/10, OS X.

---
 doc/openvpn.8         |  4 +++
 src/openvpn/forward.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/options.c | 10 ++++++++
 src/openvpn/options.h |  4 +++
 src/openvpn/proto.h   | 32 +++++++++++++++++++++++
 5 files changed, 121 insertions(+)

index d9bb77c..0decc54 100644
@@ -3796,6 +3796,10 @@ rather than waiting for a timeout.  The
 parameter (default=1) controls the maximum number of attempts that the client
 will try to resend the exit notification message.  OpenVPN will not send any exit
 notifications unless this option is enabled.
+.TP
+.B \-\-allow\-recursive\-routing
+When this option is set, OpenVPN will not drop incoming tun packets
+with same destination as host.
 .\"*********************************************************
 .SS Data Channel Encryption Options:
 These options are meaningful for both Static & TLS-negotiated key modes

index 1ef0299..0ecaad9 100644
@@ -500,6 +500,8 @@ static const char usage_message[] =
   "--server-poll-timeout n : when polling possible remote servers to connect to\n"
   "                  in a round-robin fashion, spend no more than n seconds\n"
   "                  waiting for a response before trying the next server.\n"
+  "--allow-recursive-routing : When this option is set, OpenVPN will not drop\n"
+  "                  incoming tun packets with same destination as host.\n"
 #endif
 #ifdef ENABLE_OCC
   "--explicit-exit-notify [n] : On exit/restart, send exit signal to\n"

@@ -875,6 +877,7 @@ init_options (struct options *o, const bool init_gc)
   }
 #endif /* WIN32 */
 #endif /* P2MP_SERVER */
+  o->allow_recursive_routing = false;
 }
 
 void

@@ -2089,6 +2092,8 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 	msg (M_USAGE, "--ifconfig-ipv6-pool needs --ifconfig-ipv6");
       if (options->ifconfig_ipv6_local && !options->tun_ipv6 )
 	msg (M_INFO, "Warning: --ifconfig-ipv6 without --tun-ipv6 will not do IPv6");
+      if (options->allow_recursive_routing)
+	msg (M_USAGE, "--allow-recursive-routing cannot be used with --mode server");
 
       if (options->auth_user_pass_file)
 	msg (M_USAGE, "--auth-user-pass cannot be used with --mode server (it should be used on the client side only)");

@@ -7121,6 +7126,11 @@ add_option (struct options *options,
       options->use_peer_id = true;
       options->peer_id = atoi(p[1]);
     }
+  else if (streq (p[0], "allow-recursive-routing") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->allow_recursive_routing = true;
+    }
   else
     {
       int i;

index 26b09ea..86fa8c2 100644
@@ -598,6 +598,10 @@ struct options
 
   bool use_peer_id;
   uint32_t peer_id;
+
+  /* Useful when packets sent by openvpn itself are not subject
+     to the routing tables that would move packets into the tunnel. */
+  bool allow_recursive_routing;
 };
 
 #define streq(x, y) (!strcmp((x), (y)))

index f91e787..93444dd 100644
@@ -219,6 +219,38 @@ struct ip_tcp_udp_hdr {
                              - sizeof(struct openvpn_tcphdr))
 
 /*
+ * This returns an ip protocol version of packet inside tun.
+ */
+inline static int get_tun_ip_ver(int tunnel_type, struct buffer *buf)
+{
+  int offset;
+  int ip_ver;
+  const struct openvpn_iphdr *ih;
+
+  ip_ver = -1;
+
+  /* for tun get ip version from ip header */
+  if (tunnel_type == DEV_TYPE_TUN)
+    {
+      if (likely(BLEN (buf) >= (int) sizeof (struct openvpn_iphdr)))
+	{
+	  ip_ver = OPENVPN_IPH_GET_VER (*BPTR(buf));
+	}
+    }
+  else if (tunnel_type == DEV_TYPE_TAP)
+    {
+      /* for tap get ip version from eth header */
+      if (likely(BLEN (buf) >= (int)(sizeof (struct openvpn_ethhdr))))
+	{
+	  const struct openvpn_ethhdr *eh = (const struct openvpn_ethhdr *) BPTR (buf);
+	  return (ntohs (eh->proto) == OPENVPN_ETH_P_IPV6) ? 6 : 4;
+	}
+    }
+
+    return ip_ver;
+}
+
+/*
  * If raw tunnel packet is IPv4 or IPv6, return true and increment
  * buffer offset to start of IP header.
  */

1.9.1

Your patch has been applied to the following branches

commit e9d64bc03742c96a3d7fe2a473c43d40e5ba2001 (master) commit 122469f5ad30b563cbefbc753d2a55af4227bb74 (release/2.3)

Author: Lev Stipakov
Date:   Mon Jan 4 14:43:44 2016 +0200

     Drop recursively routed packets

     Signed-off-by: Lev Stipakov <[email protected]...>
     Trac: 642
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://sourceforge.net/p/openvpn/mailman/message/34737757/
     Signed-off-by: David Sommerseth <[email protected]...>

--
kind regards,

David Sommerseth